Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PARQUET-2417: Add support for geometry logical type #2971

Open
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

zhangfengcdt
Copy link

This PR is to provide a POC to support the proposed changes to the parquet-format to add geometry type to parquet.

Here is the proposal: apache/parquet-format#240

Jira

Tests

  • My PR adds the following unit tests: TestGeometryTypeRoundTrip

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

Sorry, something went wrong.

@zhangfengcdt zhangfengcdt marked this pull request as draft July 26, 2024 15:39
@zhangfengcdt
Copy link
Author

CC: @jiayuasu @Kontinuation @wgtmac

@zhangfengcdt zhangfengcdt marked this pull request as ready for review August 12, 2024 15:57
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! I have left some comments. I think we are reaching the finish line!

}

@Test
public void testEPSG4326BasicReadWriteGeometryValue() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these tests!

I think we are missing tests in following cases:

  • verify geometry type metadata is well preserved.
  • verify all kinds of geometry stats are preserved, including bbox, covering and geometry types.
  • verify geo stats in the column index have been generated.

I can do these later.

@jiayuasu
Copy link
Member

@wgtmac please take a look :-)

@wgtmac
Copy link
Member

wgtmac commented Feb 13, 2025

Sure, I will take a look. Thanks!

@Kontinuation
Copy link
Member

I am depending on this PR to build geo support for iceberg. I got lots of test failures when building this branch locally:

java.lang.NullPointerException
	at org.apache.parquet.format.converter.ParquetMetadataConverter.fromParquetStatistics(ParquetMetadataConverter.java:965)
	at org.apache.parquet.format.converter.ParquetMetadataConverter.buildColumnChunkMetaData(ParquetMetadataConverter.java:1750)
	at org.apache.parquet.format.converter.ParquetMetadataConverter.fromParquetMetadata(ParquetMetadataConverter.java:1848)
	at org.apache.parquet.format.converter.ParquetMetadataConverter.readParquetMetadata(ParquetMetadataConverter.java:1728)
	at org.apache.parquet.hadoop.ParquetFileReader.readFooter(ParquetFileReader.java:629)
	at org.apache.parquet.hadoop.ParquetFileReader.<init>(ParquetFileReader.java:934)
	at org.apache.parquet.hadoop.ParquetFileReader.<init>(ParquetFileReader.java:925)
	at org.apache.parquet.hadoop.ParquetFileReader.open(ParquetFileReader.java:698)

NPE is thrown when reading parquet files without geo columns. Can we apply the following patch to resolve this problem?

diff --git a/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java b/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
index 3efc9345..22e51783 100644
--- a/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
+++ b/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
@@ -961,6 +961,9 @@ public class ParquetMetadataConverter {
 
   static org.apache.parquet.column.statistics.geometry.GeospatialStatistics fromParquetStatistics(
       GeospatialStatistics formatGeomStats, PrimitiveType type) {
+    if (formatGeomStats == null) {
+      return null;
+    }
     org.apache.parquet.column.statistics.geometry.BoundingBox bbox = null;
     if (formatGeomStats.isSetBbox()) {
       BoundingBox formatBbox = formatGeomStats.getBbox();

Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! Please see my inline comments.

@@ -30,6 +32,7 @@ public class BinaryStatistics extends Statistics<Binary> {

private Binary max;
private Binary min;
private GeospatialStatistics geospatialStatistics = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to leave BinaryStatistics as is without any change. This has been discussed in the spec PR that GeospatialStatistics should be similar to the implementation of SizeStatistics. Users can directly call ColumnChunkMetaData.getGeospatialStatitics() to get it. Please revert all changes in this file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at ColumnValueCollector class and follow the same approach of collecting SizeStatistics for GeospatialStatistics.

@@ -178,6 +183,30 @@ public Statistics<?> build() {
}
}

// Builder for GEOMETRY type to handle GeometryStatistics
private static class GeometryBuilder extends Builder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment in BinaryStatistics.java, we need to revert all changes in this file as well.

xMin = minX;
xMax = maxX;
} else {
// Handle the wraparound case for X values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check if it is geography type first or use a subclass for bbox of geography type? IIUC, geometry type does not need to check wraparound.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to check wraparound for geometry type.

// calculate the geometryStatistics from the BinaryStatistics
GeospatialStatistics geospatialStatistics = null;
if (currentStatistics instanceof BinaryStatistics)
geospatialStatistics = ((BinaryStatistics) currentStatistics).getGeospatialStatistics();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refactor the change to be similar to currentSizeStatistics.

}
}

class GeometryColumnChunkMetaData extends ColumnChunkMetaData {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not create another ColumnChunkMetaData subclass. Please follow what we did for SizeStatistics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants